- 
                Notifications
    
You must be signed in to change notification settings  - Fork 21.5k
 
params: Config2 #32224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
params: Config2 #32224
Conversation
It's not great to call scheduleAtTime so many times. The function is much more expensive now since it has to establish the fork order every time. A later refactoring needs to turn it around to compute the order only once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really good, just catching up a bit on the changes. Saw a TODO for the TTD and BPOs for checking compatible - are there other major todo items?
| 
               | 
          ||
| var ConfigParam = params.Define(params.T[Config]{ | ||
| Name: "clique", | ||
| Optional: true, // optional says | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing part of the comment here?
| param map[int]any | ||
| } | ||
| 
               | 
          ||
| func NewConfig2(activations Activations, param ...ParamValue) *Config2 { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc
| return ok | ||
| } | ||
| 
               | 
          ||
| // SetActivations returns a new configuration with the given forks activated. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // SetActivations returns a new configuration with the given forks activated. | |
| // SetActivations returns a new configuration with the given forks activations. | 
I think it should say "activations" since the way it currently reads is that that it will make the new config have the passed in forks be active.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is a little confusing to me in general though, it seems like it should set the forks for the current Config2 object (even overwriting the existing) but instead it returns a new object that has both the existing activations and the new activations.
| func (cfg *Config2) Rules(blockNum uint64, blockTime uint64) Rules2 { | ||
| r := Rules2{ | ||
| active: make(map[forks.Fork]bool, len(cfg.activation)), | ||
| } | ||
| return r | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this still needs to fill in the active map?
~Will probably be mostly supplanted by #32224, but this should do for now for devnet 3.~ Seems like #32224 is going to take some more time, so I have completed the implementation of eth_config here. It is quite a bit simpler to implement now that the config hashing was removed. --------- Co-authored-by: MariusVanDerWijden <m.vanderwijden@live.de> Co-authored-by: Guillaume Ballet <3272758+gballet@users.noreply.github.com> Co-authored-by: rjl493456442 <garyrong0905@gmail.com>
~Will probably be mostly supplanted by ethereum#32224, but this should do for now for devnet 3.~ Seems like ethereum#32224 is going to take some more time, so I have completed the implementation of eth_config here. It is quite a bit simpler to implement now that the config hashing was removed. --------- Co-authored-by: MariusVanDerWijden <m.vanderwijden@live.de> Co-authored-by: Guillaume Ballet <3272758+gballet@users.noreply.github.com> Co-authored-by: rjl493456442 <garyrong0905@gmail.com>
~Will probably be mostly supplanted by ethereum#32224, but this should do for now for devnet 3.~ Seems like ethereum#32224 is going to take some more time, so I have completed the implementation of eth_config here. It is quite a bit simpler to implement now that the config hashing was removed. --------- Co-authored-by: MariusVanDerWijden <m.vanderwijden@live.de> Co-authored-by: Guillaume Ballet <3272758+gballet@users.noreply.github.com> Co-authored-by: rjl493456442 <garyrong0905@gmail.com>
This is a refactoring of the fork configuration system. The design goals for this change are as follows:
Forks
to make this simpler, so that ideally a new enum value has to be added and that's it.
Parameters
handled by a global registry, and can be implemented across different packages. For example, the EIP-1559 configuration can be implemented in the eip1559 package, where the config is ultimately used.
Config/Rules object
map[Fork]bool.